-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ui: Namespace Support #6639
ui: Namespace Support #6639
Conversation
76a7919
to
144686e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬
SourceType: serialized.SourceType, | ||
Action: serialized.Action, | ||
Description: serialized.Description, | ||
}} | ||
`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means we can easily prevent certain props/values from being sent in a request, and means we can get rid of our custom writable
function. We only did it for intentions for now as we were there, but we'll probably remove elsewhere in another PR and eventually delete our custom writable
. You'll see us removing this for the intentions model/serializer further down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you use serializer at all with the new adapter? Just curious as in out-of-the-box ember data, that's where this would be done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use our old style ApplicationSerializer, but we don't normalize anything there, the serializer deals with all the HTTP header bits still but not a lot else. Long term we hope to move everything to this style of normalization as its a easier to see whats happening, and looks exactly like the Consul API documentation, um more notes here:
We still plan on a similar refactoring of our Serializers (similar to what we did for our Adapters) for response serializing, just its not high on the priority list right now, it might happen pre-1.8 or something.
.replace('/root-create', '/create') | ||
.replace('/create', '/edit') | ||
.replace('/folder', '/index'), | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if we could read the templateName
property on the Route before reopening it. Kind of like const currentTemplate = Route.prototype.templateName
. If anyone knows how to do that I'd be very grateful. Otherwise I'm guessing once we move to Octane/JS classes we should be able to just inspect the prototype.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you may be able to do route.proto()
to get a reference to the prototype.
container | ||
.lookup('service:dom') | ||
.root() | ||
.classList.add('has-nspaces'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use this for CSS 'is this namespaced or not' styling.
'SourceType', | ||
'Description', | ||
]); | ||
export default model; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heres where we get rid of writable
routeParams.nspace = nspacedRoute.params.nspace; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've mentioned this code ^ offline before now. This seems to be the only way to reliably get a part of the location.hash
from outside of a Route
. It will also be the place where we eventually write/read from localStorage
for when we have the ui/setting
URL which does't include the namespace, similar to how we already do with datacenters.
const nodes = payload.Nodes; | ||
const service = payload.Nodes[0]; | ||
service.Nodes = nodes; | ||
service.Tags = [...new Set(payload.Nodes[0].Service.Tags)]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We realized that we weren't uniquing the Tags here, yet we are in the application. We've also made a note in the application source to move from uniq
to Set
.
This has become noticeable now as faker
's random-but-not-randomness can change slightly when you add calls to faker
. We've added a tonne of more calls to faker for our namespace mocking so this started to fail without this change.
9429b2a
to
fe7ed39
Compare
e8ca326
to
56054ef
Compare
45ff66a
to
b043d57
Compare
40e528f
to
b1314a5
Compare
38b3083
to
5c7f06e
Compare
client: service('client/http'), | ||
formatNspace: function(nspace) { | ||
if (config.CONSUL_NSPACES_ENABLED) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the enterprise binary write this value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will do at somepoint yeah, I think for now we can just use our CONSUL_BINARY_TYPE setting that we already have - I think this specific NSPACE one is going to be added later. Potentially a PR coming after this one to temporarily use the CONSUL_BINARY_TYPE setting (which the binary writes already)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
client: service('client/http'), | ||
formatNspace: function(nspace) { | ||
if (config.CONSUL_NSPACES_ENABLED) { | ||
return nspace !== '' ? { [NSPACE_QUERY_PARAM]: nspace } : undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need to be url encoded? wasn't sure if request
does that for you or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah request
encodes every variable you pass inside the template literals, both keys and values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't strictly correct 😁 . Basically its not request
that does it its ember-data/jQuery.
When writing the template literal any variables used in the URL itself is encoded. Then any body used in anything other that a GET request is sent as JSON encoded.
A GET request with a body is a little special and follows the old ember-data adapter functionality, in that any body with a GET request gets moved to the GET params (as you can't send a body with a GET request). Under the hood ember-data relied on jQuery to do this and when we did our data layer refactor we tried to replicate this functionality completely, you can see it here with a comment on urlencoding https://github.com/hashicorp/consul/pull/5637/files#diff-d12a13b0f2b1e3556993c370c1ee3ddfR182.
I've just double checked this and it does get url encoded, but we are also protected somewhat by the fact that namespaces can only have DNS compatible names, and therefore probably don't need url encoding anyhow
} | ||
this._super({ | ||
...model, | ||
...{ | ||
item: this.form.setData(model.item).getData(), | ||
SourceName: source, | ||
DestinationName: destination, | ||
SourceNS: sourceNS, | ||
DestinationNS: destinationNS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do these just get ignored by the OSS binary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so yeah, we've tested it against the binary and everything works a-ok. @mkeeler just to double check if I POST these values to OSS they are noop's right?
@@ -0,0 +1,2 @@ | |||
import Controller from './edit'; | |||
export default Controller.extend(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're not doing anything here, you can remove this file and ember will create it automatically for you. No harm in leaving it either, just thought I'd mention it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think we'd usually delete these, but this one needs to be here in order to extend ./edit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha thanks, I totally missed that that's what was happening, that answers another question I had too.
ui-v2/app/helpers/href-mut.js
Outdated
// 'ideally' we could try and do this elsewhere | ||
// not super important though. | ||
// This will turn an URL that has no nspace (/ui/dc-1/nodes) into one | ||
// that does have a namespace (/ui/~nspace/nodes) if you href-mut with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this right that the nspace replaces the datacenter in the route? (sorry I know that this is a comment, but I wasn't sure of the interplay of namespaces and datacenters).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhat related: I know nested namespaces aren't supported at this time - is that expected in the future? will the format then be with slashes and url encoding will come into play here (or somewhere around here)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good spot, theres a typo here which makes the comment confusing in this context! Will tweak, thanks!
I know nested namespaces aren't supported at this time - is that expected in the future?
I think its expected yeah, but I think we covered this future potential in RFC-land (we'll either encode them or switch them out for another non-DNS character)
container.register(`controller:nspace/${item}`, controller); | ||
} | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ends up doubling a bunch of registrations - have you seen any issues with that? I think there's no way to get to the non-namespaces registrations via normal app interfaces since you're re-writing route names for links, but didn't know if it'd cause other weirdness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope not seen any issues. If anything I'm guessing we could get issues with sticky queryParams here (it may stick to both), but we aren't use those anywhere as yet.
I think there's no way to get to the non-namespaces registrations via normal app interfaces since you're re-writing route names for links
From what I remember we only rewrite the route names if you are in a namespace, if you aren't you just get normal routes.
@@ -10,7 +11,7 @@ export const routes = { | |||
// Our parent datacenter resource sets the namespace | |||
// for the entire application | |||
dc: { | |||
_options: { path: ':dc' }, | |||
_options: { path: '/:dc' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change breaking? should there be a redirect from non-slash to slash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think both of these are the same, so the functionality remains the same.
It's actually not got a lot to do with the PR, but I have here so I made it all consistent with the rest of the app, I'd guess this slashless version is probably copied from the old v1 UI.
export default Route.extend(WithNspaceActions, { | ||
repo: service('repository/nspace'), | ||
isCreate: function(params, transition) { | ||
return transition.targetName.split('.').pop() === 'create'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does create end up on the edit route?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The create route extends the edit route, so they are both the same, but occasionally we need to know if its one or the other.
@@ -52,6 +57,7 @@ export default Service.extend({ | |||
}); | |||
}, | |||
invalidate: function() { | |||
// TODO: This should probably return a Promise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the underlying methods do (yet) - there was some talk of changing the ember data api to do this, but I don't remember where it landed / what progress there was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah for sure, this was more a TODO to make my Repository class methods all use a more consistent Promise based interface. Most of the methods return Promises, so I would like these ones to also.
%main-nav-horizontal-drop-nav div { | ||
max-width: fit-content; | ||
} | ||
@supports not (max-width: fit-content) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so fancy - we should do that whole "officially don't support IE 11" thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hehe, yeah I think we chatted through this a bit a while back, this was a fun one to do. I think it still looks ok if the browser doesn't support any of this, its just not perfect.
Amongst the team we are kinda 'this is ok' right now, but yeah I'd definitely be up for officially saying something somewhere.
} | ||
%main-nav-horizontal-drop-nav div { | ||
padding: 10px; | ||
padding-left: 36px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason for mixing rem
and px
and em
in this file? would be nice to keep it to one unit if possible, but I realize odd px values causes weird decimals sometimes in rems/ems .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Generally' I try and use em
for tops and bottoms, and then percentages for lefts/rights. That way you keep some sort of vertical rhythm (hence the top/bottom ems) and percentage/window width based based left/right responsiveness.
Consul UI isn't super strict with this, just due to the way we initially built it, and we never really got around to doing proper percentage based responsiveness so there is a lot of px
and there is no spec for the vertical rhythm.
I'm guessing these rems are just copied over from what we had before here. I am intrigued as to why we went with rem
here and not em
though. I'll have a look and see, if its just what we had before though I'd keep it the same for the moment. All of the CSS changes here were trying to avoid changing things and just organizing/refactoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah so just took a quick look, this is definitely new. I think it's related to text and being able to judge the width of the menu that we use that @supports
for. I'm gonna take another look though as I'm wondering why I went with rem
and not em
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OMG - so, this is such an amazing deep dive! 😆
Recently we made these things called %internal-button
s in https://github.com/hashicorp/consul/pull/6495/files#diff-9ab79d006f089e09cd9e901c83788d3fR41 which are basically buttons within menu panels (as they don't use the border/background color the same as other buttons), and I traced the rem
usage back to these (you can see the 0.75rem 1rem
in that PR link above).
The rem
s you see in this file reuse the 1rem
left and right padding of these buttons as the left and right padding of the header needs to be the same as the %internal-button
s below it:
See there ^ how the left edges of the buttons and the header (NAMESPACES
) all line up exactly.
The top and bottom padding will have been 'make this look like the designs' which is probably why we use the 0.2rem
and 0.7rem
for top and bottom, I probably just tweaked this until it looked right/lined up with the designs.
So the big question is, why rem
's, I wouldn't have usually chosen those?
So the %internal-buttons
came about when we were moving our menu styling to more closely follow structure and while I was making sure they would work with @joshuaogle s slide-y confirmation dialogs in the menus, so followed the codepen for these as closely as possible - which was what made me think I wonder what the units where in that codepen? /me come on browser history don't let me down
0.75rem 1rem
! Amazing!
(Sorry maybe it just me that loves this 😆 )
I'll probably find the em
/px
equivalent at some point in another PR, I'm just gonna drop a TODO comment in for now.
@@ -0,0 +1,2 @@ | |||
<input type="checkbox" id={{concat 'toggle-button-' guid}} onchange={{action 'change'}} /> | |||
<label data-test-toggle-button={{name}} for={{concat 'toggle-button-' guid}}><a href="#" onclick={{action 'click'}}>{{yield}}</a></label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you do a button instead of an href-less anchor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh possibly! We just ignore the click, this is only so you can tab to the link still even though it's a label. I suppose it's not a hyperlink, so a button would probably make more sense, will take a look 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the need for tabbing to the label? If you just need to put it in the tab order of the document you could add tabindex="1"
to the label
, or if you just need to make it programmatically focusable, you could make add tabindex="-1"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah so bit of extra explanation here, this is the Datacenter and Namespace menu trigger (you click this to open/close the dropdown), so ideally you need to be able to tab to it (along with all the other things in the main nav menu). I think generally folks shy away from using positive tabindex
s, it's one of those things that once you start using it rapidly becomes unmanageable. So we want it to remain in the natural tabbing order right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d'oh I meant "0" for the first one, you're totally right about shying away from positive tab indicies. Is the checkbox in the tab order? I wonder if moving to a button will allow usage of spacebar for toggling instead of enter (looks like enter is what triggers it in chrome).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<button>
along with the spacebar works! Superstar! Thankyou!
P.S. I think I'm gonna add a bunch of roles/aria attributes here and potentially pass the button in via the yield
so I'm gonna save this for another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #6941
@@ -11,7 +11,13 @@ | |||
{{/block-slot}} | |||
{{#block-slot 'row'}} | |||
<td> | |||
<span data-test-destination-name>{{item.DestinationName}}</span> | |||
<a data-test-destination-name> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this an anchor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From memory I think I noticed we were using a a
elsewhere, and I'm pretty sure we are going to link these very soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😅 wow that's a chunk of work - nice one @johncowen. Mostly questions from me - there were a couple of things about <a>
tags that I was wondering about, but that's the only thing I noticed going through all of this. Probably easier to fix those after this is in, but will leave it up to you.
6830f7c
to
3e381a7
Compare
5c7f06e
to
bdb2e01
Compare
Codecov Report
@@ Coverage Diff @@
## ui-staging #6639 +/- ##
==============================================
+ Coverage 65.62% 65.64% +0.01%
==============================================
Files 443 443
Lines 53312 53312
==============================================
+ Hits 34988 34994 +6
+ Misses 14098 14095 -3
+ Partials 4226 4223 -3
Continue to review full report at Codecov.
|
This is the same fix as #6555 but for the 1.7 version of the UI with additional nspace support
This may cause tests to fail which where asserting the first HTTP request on page navigation as now every single page at least calls /v1/catalog/datacenters first
This is possibly temporary
* Adds `href-mut` helper for modifying current URLs Sometimes you just need to link to your current route, but alter one of the parameters of the route. In our case the datacenter menu is a good example. When you change the datacenter from the node listing page we want the user to be taken to the node listing page on the newly selected datacenter, not a different route. This commit uses the router service to walk up the current route and replace parameters in the route that you specify as an argument to the helper. This then uses the newly mutated arguments and the existing routeName to pass through to the already existing hrefTo helper.
We also removed %with-proxy here which is no longer being used
af3750c
to
fce23da
Compare
Integrate and use the `index.html` variables passed through form the binary to the UI to understand whether namespaces and ACLs are enabled. Also use the new internal `acl/authorize` endpoint to understand whether the user has permissions to access the namespace management.
Adds namespace support to the UI: 1. Namespace CRUD/management 2. Show Namespace in relevant areas (intentions, upstreams) 3. Main navigation bar improvements 4. Logic/integration to interact with a new `internal/acl/authorize` endpoint
Hey there, This issue has been automatically locked because it is closed and there hasn't been any activity for at least 30 days. If you are still experiencing problems, or still have questions, feel free to open a new one 👍. |
This PR begins to add Namespace support to the UI.
Preview link
Related work:
The majority of how this is done has been detailed previously offline and we've tried to split the commits up so it easier to see what's happening here.
The majority of this is usual ember, but as always there's a catch 😄 . The majority of anything out of the ordinary we've kept in this commit 8029d22.
Some notes:
ns
argument to support ans=
query parameter. If this argument is undefined, it does not get added to the query parameter. Sometimes we have{ ns: "" }
in which case we convert that toundefined
. You'll see this inApplicationAdapter.formatNspace
.something.dot.something
) Will automatically figure out if they are at a URL with a namespace in it, if they are it is converted in the background to use that. You can maybe think of this as 'relative' route names. Sodc.services
will be converted in the background tonspace.dc.services
if you are at a URL with a/~namespace/
in it. This can be seen in 8029d22."default", "team-1", undefined
for all the previous tests.%menu-panel
here ui: CSS Upgrade (action-group,form-elements,sliding-toggle,breadcrumbs) #6495 once thats available.All of this is only available in Consul Enterprise, so we include a config value to turn all of this on and off. This has partly been explained offline so we won't touch on that here. But this work hasn't quite been finished yet and is dependent on backend work.
Being able to switch further functionality dependent on whether ACLs are enabled or not is also awaiting backend support, plus understanding the Namespace that a users ACL originates from is also awaiting backend support. This affects 'remembering' your current Namespace for which we will potentially use the same approach as we currently do for datacenters and is mainly require for showing your current namespace on the Settings page. Ideally we'd probably move the URL for the settings page so it includes the name of the dc and nspace. But this is work for the future.
Acceptance tests specifically for Namespace CRUD aren't added as yet, they will be added before final master merge
None of the awaiting work should block a potential approval here:
ui-staging
and we'll probably add the remaining work once the backend support is finished in 2 - 3 weeks time. In the meantime we'll need the work here to continue with Discovery Chain work.I might add a self review in here with some notes for helping reviews.